Add LayerSet::get_or_create_layer
#291
Conversation
|
I think I removed this method in #255, also see the reasoning. If you come up with something robust, I'd want it back in :) |
|
Haha didn't realise it was originally in the library but removed. I have a use for it and have basically implemented a worse version in my own code (since I don't have access to |
|
If you can create something that deals with the pain points we found earlier, certainly! We can look at it together. |
|
I can't really see any pain points from previous discussion other than "we don't use this" and "this method is getting ugly/complex". The latter I think I've solved by pulling out the common logic from Let me know if I've missed anything, because currently I don't understand what's wrong with the method or my implementation of it |
There was a problem hiding this comment.
So one of the reasons we got rid of this was that we weren't using it, and I don't like holding onto API that isn't being used. If you are using it, though, then that is no longer a concern.
In any case, the new method here feels much less of a wart than the previous one, I think because we didn't have NamingError originally?
I have one note inline, but I'm happy to merge this, and then I'll get a release out when that's done. :)
src/layer.rs
Outdated
| match index { | ||
| Some(its_here) => Ok(&mut self.layers[its_here]), | ||
| None => { | ||
| // We can avoid the formal checks that new_layer does because by checking if it's already in the list, |
There was a problem hiding this comment.
I think we still want to check that we aren't DEFAULT_LAYER_NAME, since it's possible that the default layer has another name.
See this test case that was deleted when we removed the previous version:
#[test]
#[should_panic(expected = "reserved")]
fn get_or_create_false_default_layer() {
let mut layer_set = LayerSet::default();
layer_set.rename_layer("public.default", "foreground", false).unwrap();
layer_set.get_or_create("public.default");
}and... I think honestly it's easiest/hardest to screw up if t his does just call the old new_layer method if we find nothing. In practice this isn't called much, and iterating even small-10s of layers is still going to be very fast, and then we can be confident we're always using the same logic.
There was a problem hiding this comment.
Addressed in latest commit
At the cost of some efficiency (now iterates through layers twice)
|
Changed |
|
Thanks! |
|
(I've release 0.9.0) |
This one's a pain to do intuitively because of the borrow checker
You want to do something like:
But the borrow checker won't let you because of multiple mutable borrows (which is arguably incorrect since
Noneshouldn't be considered tied toufo.layers, but improving the borrow checker is beyond my skill)So what I've implemented is accepted by the borrow checker and doesn't iterate through
layersmultiple times. I've pulled out the happy path ofnew_layerto avoid the code duplication, hence the creation of the private functioncreate_layer. Bikeshedding the naming is welcome because it's definitely not amazing, but I just wanted to get the PR out there and worry about naming later